Add preflight and delete option in sessions handler #363
Conversation
|
@blink1073 or @lresende, I'm hoping one of you could take a look at this PR. I just don't know enough about this aspect of the stack to know if this introduces any issues or not - sorry about that. (Thank you) |
|
I haven't actually tested but did a little research on pre-flight calls and the code looks good. |
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #363 +/- ##
==========================================
- Coverage 95.78% 95.66% -0.12%
==========================================
Files 31 31
Lines 2252 2260 +8
==========================================
+ Hits 2157 2162 +5
- Misses 95 98 +3
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
|
Hi @damiles - I know it's been "forever" since you opened this PR and we apologize for not being more active on this repository. I'd like to get this PR merged (and released via a patch release Are you able to try out the changes we've been discussing that I just pushed? Since the notebook/jupyter server gateway client doesn't use the Session API when interacting with the Gateway server, I suspect you must have your own frontend that this PR is addressing. |
|
@kevin-bates Don't worry, I will test it on my own frontend as soon as possible. |
|
Awesome - thank you @damiles! Please let us know how it goes and we'll cut a release as soon as this PR is merged. |
| def set_default_headers(self): | ||
| self.set_header('Content-Type', 'application/json') | ||
| self.set_header('Access-Control-Allow-Origin', self.settings['kg_allow_origin']) | ||
| self.set_header('Access-Control-Allow-Headers', self.settings['kg_allow_headers']) | ||
| self.set_header('Access-Control-Allow-Methods', self.settings['kg_allow_methods']) | ||
|
|
||
| def options(self, **kwargs): | ||
| """Method for properly handling CORS pre-flight""" | ||
| self.finish() |
There was a problem hiding this comment.
After working on #384, I'm curious why the existing CORSMixin isn't already sufficient, since it adds everything that is listed here, except Content-Type.
Are the bases of the else statement below not doing what is intended?
When try to call a delete session, /api/sessions/<session_id> we obtain a preflight error and cors. See screenshots for more info:
I solve adding a new handler and allowing options call for preflight and adding delete options in headers. (Maybe is not all correct for integration because I don't read full kernel_gateway code) But this works fine and solve the issue.
This issue looks like appears in #297 too.